Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[5.0] avoid using a stack variable after return #2309

Merged
merged 2 commits into from
Mar 23, 2024
Merged

[5.0] avoid using a stack variable after return #2309

merged 2 commits into from
Mar 23, 2024

Conversation

spoonincode
Copy link
Member

http_max_response_time is a variable declared on that stack as part of plugin_startup()

leap/plugins/producer_api_plugin/producer_api_plugin.cpp

Line 94 in c301152

fc::microseconds http_max_response_time = http.get_max_response_time();

CALL_WITH_400 captures all by reference

leap/plugins/producer_api_plugin/producer_api_plugin.cpp

Lines 23 to 26 in c301152

#define CALL_WITH_400(api_name, category, api_handle, call_name, INVOKE, http_response_code) \
{std::string("/v1/" #api_name "/" #call_name), \
api_category::category, \
[&](string&&, string&& body, url_response_callback&& cb) mutable { \

INVOKE_R_R_D then goes on to use the reference to http_max_response_time which is stale as plugin_startup() has long since returned once HTTP requests are made

leap/plugins/producer_api_plugin/producer_api_plugin.cpp

Lines 65 to 66 in c301152

#define INVOKE_R_R_D(api_handle, call_name, in_param) \
auto deadline = http_max_response_time == fc::microseconds::maximum() ? fc::time_point::maximum() \

This problem seemed to have been introduced in 5.0. Since this is in producer_api, which isn't expected to ever be exposed publicly, not considering this a security defect.

heifner
heifner reviewed Mar 13, 2024
View reviewed changes
Copy link
Member

@heifner heifner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it would be safer to capture all by value = in CALL_WITH_400 instead of all by reference &.

leap/plugins/producer_api_plugin/producer_api_plugin.cpp

Lines 23 to 26 in c301152

#define CALL_WITH_400(api_name, category, api_handle, call_name, INVOKE, http_response_code) \
{std::string("/v1/" #api_name "/" #call_name), \
api_category::category, \
[&](string&&, string&& body, url_response_callback&& cb) mutable { \

@ericpassmore
Copy link
Contributor

Note:start
group: CLEANCODE
category: INTERNALS
summary: Cleanup usage of stack variables in producer api plugin.
Note: end

@spoonincode
Copy link
Member Author

safer

Well, personally for non-trivial usages of lambdas -- such as this case -- I like to see explicit capture lists. So [&producer, &http] in this case. Does that work?

@heifner
Copy link
Member

heifner commented Mar 22, 2024

safer

Well, personally for non-trivial usages of lambdas -- such as this case -- I like to see explicit capture lists. So [&producer, &http] in this case. Does that work?

Sounds good.

@spoonincode spoonincode merged commit f882aec into release/5.0 Mar 23, 2024
29 checks passed
@spoonincode spoonincode deleted the papip_stack_usage_50 branch March 23, 2024 03:24
@spoonincode spoonincode mentioned this pull request Mar 23, 2024
Merged
@spoonincode
Copy link
Member Author

that was probably a bad idea since now clang warns of unused lambda captures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg7mdp greg7mdp greg7mdp approved these changes

@heifner heifner heifner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@spoonincode @ericpassmore @heifner @greg7mdp